✅ Deploy Preview for nextflow-docs-staging canceled.

@claude please review

Claude finished @bentsherman's task —— View job

Code Review: Enable strict syntax by default

Reviewing the changes to enable strict syntax (v2 parser) by default and deprecate the nextflow.enable.strict feature flag.

Review Tasks:

• Understand the scope and impact of the changes
• Review core implementation changes (NF.groovy, ConfigBuilder, etc.)
• Examine parser and script loading modifications
• Review test consolidation and ScriptHelper implementation
• Check documentation updates
• Analyze potential breaking changes and migration path
• Provide comprehensive feedback

Review Summary

This is a major but well-executed change that enables strict syntax by default in Nextflow. The implementation is thorough and the extensive test updates demonstrate careful attention to maintaining compatibility while enforcing stricter parsing rules.

Positive Aspects

1. Clean Implementation of Default Change

• NF.groovy:36: The change from 'v1' to 'v2' as default for NXF_SYNTAX_PARSER is clean and well-placed
• ConfigBuilder.groovy:440-441: Appropriate conditional validation only applies to v1 parser, maintaining backward compatibility
• The change is controlled by environment variable, allowing easy rollback if needed

2. Comprehensive Test Consolidation

• ScriptHelper.groovy: Excellent consolidation of test harness functionality that was scattered across multiple helper classes
• The new ScriptHelper provides unified methods for loadScript(), runScript(), and parseAndReturnProcess()
• Proper session management with MockSession and MockExecutor classes
• Smart result normalization handling ChannelOut and dataflow constructs

3. Documentation Updates

• docs/strict-syntax.md: Clear versioning annotation showing v2 is now default (line 19-21)
• docs/reference/feature-flags.md: Proper deprecation notice for nextflow.enable.strict (lines 25-27)
• Good migration guidance provided for users

4. Extensive Test Coverage

• 64 files modified shows comprehensive test updates
• Tests properly use the new ScriptHelper methods instead of scattered approaches
• Proper handling of syntax differences between v1 and v2 parsers

Areas for Improvement

1. Error Handling in ScriptLoaderV2

// ScriptLoaderV2.groovy:54-55
if( name )
    throw new IllegalArgumentException("The `-entry` option is not supported with the strict syntax -- use a param to run a named workflow from the entry workflow")

Concern: This error message could be more helpful. Consider:

• Providing a concrete example of how to use params instead
• Linking to documentation about strict syntax entry workflows

2. Configuration Parser Strictness

// ConfigParserV2.groovy:48
private boolean strict = true

Concern: The v2 parser defaults to strict mode, but this could cause issues for users with complex config files that haven't been updated yet. Consider:

• Adding a grace period with warnings before making it strictly enforced
• Providing clearer error messages when strict parsing fails

3. Missing Migration Path for Complex Cases

Concern: While documentation covers many cases, some complex Groovy patterns in configs might need more guidance:

• Dynamic config generation patterns
• Complex closure-based configurations
• Third-party library usage in configs

4. Test File Organization

- modules/nextflow/src/test/groovy/nextflow/extension/SplitFastqOp2Test.groovy (DELETED)
- modules/nextflow/src/testFixtures/groovy/test/MockHelpers.groovy (DELETED)  
- modules/nextflow/src/testFixtures/groovy/test/TestParser.groovy (DELETED)

Observation: Good cleanup of obsolete test utilities, but ensure all functionality is preserved in the new ScriptHelper.

Technical Implementation Review

1. Parser Selection Logic

The implementation correctly uses environment variable to control parser selection:

// NF.groovy:36
static String getSyntaxParserVersion() {
    return SysEnv.get('NXF_SYNTAX_PARSER', 'v2')
}

This provides a clean escape hatch for users who need v1 behavior.

2. Backward Compatibility

The changes maintain good backward compatibility:

• V1 parser still available via environment variable
• Existing validation only applies to v1 parser
• Gradual deprecation path for feature flags

3. AST Transformations

The visitor pattern changes in nf-lang module look appropriate for handling the stricter syntax requirements while maintaining compatibility with existing Groovy AST processing.

Recommendations

1. Consider Phased Rollout

• Consider adding a "strict mode warning" phase before full enforcement
• Provide migration tooling to help users identify issues in their pipelines

2. Enhanced Error Messages

• Improve error messages in ScriptLoaderV2 to be more actionable
• Consider providing suggested fixes for common migration patterns

3. Documentation Enhancements

• Add troubleshooting section for common strict syntax issues
• Provide more examples of complex config migrations
• Document performance implications of strict vs non-strict parsing

4. Monitoring Plan

• Consider adding telemetry to understand adoption and failure patterns
• Plan for support channels during the transition period

Breaking Changes Assessment

Impact Level: Medium to High - This is a breaking change for some users, but:

✅ Mitigated by:

• Environment variable escape hatch (NXF_SYNTAX_PARSER=v1)
• Comprehensive documentation and migration guide
• Clear deprecation timeline

⚠️ Potential Issues:

• Existing pipelines with complex Groovy constructs may break
• Config files using unsupported syntax patterns will need updates
• Some community pipelines may need updates

Overall Assessment

Recommendation: APPROVE

This is a well-implemented and necessary step forward for Nextflow. The change:

• Improves language consistency and tooling capabilities
• Provides clear migration path and backward compatibility
• Has comprehensive test coverage and documentation
• Sets foundation for future language improvements

The extensive test modifications (2072 additions, 3888 deletions) indicate thorough validation of the changes across the codebase.

Suggested Follow-up Actions

1. Monitor community feedback and common migration issues
2. Consider creating migration tooling for common patterns
3. Prepare FAQ for common strict syntax questions
4. Plan communication strategy for the broader community

Related: A check on how many nf-core pipelines will break when we release this 😅

https://github.com/ewels/strict-syntax-health

Better let them know it's coming!

That's the idea! Can't do much without #6623 being fixed though.

How does the timing look for merging? Is it worth moving the above fix out into its own PR for a quicker merge? We can't make the template strict-syntax compliant without that, which means that no nf-core pipelines can update until it's fixed.

I think this PR mixes two things: stabilising/improving the strict parser + promoting v2 as default

These goals are fundamentally intertwined. Making v2 the default makes it so that all of the unit tests use the v2 parser, which surfaces any remaining gaps.

For example, #6623 would have been caught by a unit test in ConfigBuilderTest if it used the v2 parser. To fix this issue by itself, we would need to duplicate the unit test or change it to use the v2 parser. But we're already planning to make v2 the default in 26.04 , so we might as well make all unit tests use the v2 parser.

So I see little difference between merging the two changes together vs separately. Battle-testing is what edge is for. We're already planning to make v2 the default in 26.04 so we might as well do it now

Whilst it scares me, I generally agree with Ben here..

battle test it into an edge

In order to really battle test it in an edge release, it needs to be the default in the edge release.

Why? would not be enough to enable v2 in the nf-core tests suite?

We can do that (though it's not trivial as it requires a template update, and we try not to push these too frequently), but I'm more thinking about people beyond nf-core.

If it's going to be default in the next stable release, I'd prefer to make it default in an edge release ASAP and then shout about it a lot. There's no way that this isn't going to cause some disruption when it comes out, so the longer it sits in an edge release the better.

though it's not trivial as it requires a template update

Really, for setting a env variable?

We can accelerate the cadence of edge releases for testing this more frequently, but from my point of view the switch should be done when there's any code/structural change.

Really, for setting a env variable?

Yes, because we would need to update the GitHub actions workflow scripts to do this. That involves a change to the pipeline template, which needs a release of tools, automatic sync, and for every pipeline to fix template merge conflicts, merge the automated template update PR, and do a release of their pipeline. This process typically takes several months from start to end.

We have talked about switching to using centralised / shared GitHub actions scripts, but this hasn't happened yet. And if we did that it probably wouldn't go down well, as flipping on the ENV var would immediately break all CI and no-one would be able to merge any PRs.

Releasing it as default in an edge release avoids all of this. We already have CI jobs in all pipelines that run on edge, don't block PRs but do alert authors to the fact that there is an upcoming breaking change.

@ben, regarding the ConfigBuilder issue I commented to you yesterday in the call, this is the part of code I was referring to:

This is using NF.strictMode, which currently only reads the value of the strict feature flag. But in CmdRun, the flag is set after creating the config. So, it will never apply the strict mode at this point.

I think NF.isStrictMode() should be implemented as follows to ensure strict mode is applied either when using v2 parsing or using the feature flag.

    static boolean isStrictMode() {
        NextflowMeta.instance.isStrictModeEnabled() || isSyntaxParserV2()
    }

Apart from my previous comment, I would also be in favor to merge and release in an edge. In case we get terrible feedback, the changes seem to be valid for both parsers, so reverting to V1 again will be just modifying the line to set the default again to V1 and the corresponding docs.

This is using NF.strictMode, which currently only reads the value of the strict feature flag. But in CmdRun, the flag is set after creating the config. So, it will never apply the strict mode at this point.

@jorgee it's fine, the strict check in mergeMaps is not needed with the v2 parser. It will just be removed when we remove the strict feature flag

We are now tracking strict syntax errors across all nf-core pipelines / modules / subworkflows:

https://github.com/nf-core/strict-syntax-health

(thanks a ton @ewels !)

I will continue to monitor the results and do what I can to ease the transition. I can make sure that error messages are clear, but at the end of the day it's up to pipeline developers to update their code. They can always set NXF_SYNTAX_PARSER=v1 to buy more time.

Looking over the error reports, I think most pipelines can be brought into compliance without too much hassle.